feat(image-adapter): improve error handling and status codes#893
feat(image-adapter): improve error handling and status codes#893iDVB wants to merge 9 commits intoopennextjs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: fdc1f4b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
vicb
changed the title
vicb
left a comment
vicb
left a comment
There was a problem hiding this comment.
Could you please add a changeset? (pnpm changeset)
Thanks!
|
Just an update. This seems to be working the dream in our prod environment so far. |
conico974
left a comment
conico974
left a comment
There was a problem hiding this comment.
Sorry to be this nitpicky on this one, but that should be the last round of change
| // Main handler logic with clearer error paths | ||
| try { | ||
| // Case 1: remote image URL => download the image from the URL | ||
| // EXTERNAL IMAGE HANDLING |
There was a problem hiding this comment.
| // EXTERNAL IMAGE HANDLING | |
| // remote image URL => download the image from the URL |
| // Download image from S3 | ||
| // note: S3 expects keys without leading `/` | ||
|
|
||
| // INTERNAL IMAGE HANDLING (S3) |
There was a problem hiding this comment.
| // INTERNAL IMAGE HANDLING (S3) | |
| // local image => download the image from the provided ImageLoader (default is S3) |
| } | ||
| const message = "Empty response body from the S3 request."; | ||
| const clientError = new IgnorableError(message, 400); | ||
| error("Empty response from S3", clientError); |
There was a problem hiding this comment.
| error("Empty response from S3", clientError); | |
| error("Empty response from ImageLoader", clientError); |
|
|
||
| // Special handling for S3 ListBucket permission errors | ||
| // AWS SDK v3 nests error details deeply within the error object | ||
| const isListBucketError = |
There was a problem hiding this comment.
All the S3 related stuff should not be here. https://github.com/opennextjs/opennextjs-aws/blob/main/packages/open-next/src/overrides/imageLoader/s3.ts
This is the ImageLoader and where you should handle S3 related error, not here.
Here we should only handle our error, or basic error
|
|
||
| // Still include error details in headers for debugging only | ||
| const errorMessage = isListBucketError ? "Access denied" : message; | ||
| res.setHeader("x-nextjs-internal-error", errorMessage); |
There was a problem hiding this comment.
Does this one propagates to the end user ?
| return buildSuccessResponse(result, options?.streamCreator, etag); | ||
| } catch (e: any) { | ||
| error("Failed to optimize image", e); | ||
| // All image-related errors should be treated as client errors |
There was a problem hiding this comment.
Not true, if something fails on the server it is not client errors
|
Did this ever make it into the build? We see this build is broken now where it was working before... There was a fix in the image server function that properly handled trailing querystring which now seem to be broken in this build now. (was working in this build less than a week ago) # PROD: WORKS
/_next/image?w=384&q=75&url=https://images.ctfassets.net/s5qqrp96y1p4/7wHwJSvxFWpIYdm56bOdSk/1208a00c3e9ed4ad32102e84d494860c/IDX_Article_-_Cultural_Forces_5_IDX_HEADER__1_.png
# PROD: WORKS
/_next/image?w=384&q=75&url=https://images.ctfassets.net/s5qqrp96y1p4/7wHwJSvxFWpIYdm56bOdSk/1208a00c3e9ed4ad32102e84d494860c/IDX_Article_-_Cultural_Forces_5_IDX_HEADER__1_.png
---
# DEV: WORKS
/_next/image?w=384&q=75&url=https://images.ctfassets.net/s5qqrp96y1p4/7wHwJSvxFWpIYdm56bOdSk/1208a00c3e9ed4ad32102e84d494860c/IDX_Article_-_Cultural_Forces_5_IDX_HEADER__1_.png
# DEV: NOT WORK
/_next/image?w=384&q=75&url=https%3A%2F%2Fimages.ctfassets.net%2Fs5qqrp96y1p4%2F7wHwJSvxFWpIYdm56bOdSk%2F1208a00c3e9ed4ad32102e84d494860c%2FIDX_Article_-_Cultural_Forces_5_IDX_HEADER__1_.png |
…e optimization failures
…h between client and server errors
…des and error classification
…h unified error classification
|
@iDVB I rebased and pushed some changes that i requested. Could you test it please ? |
|
@iDVB would you have time to test this? |
4 participants
Moved to new PR against a non-main branch.
Original PR here: #886
Fixes #885